[MNG-8696] PathModularizationCache needs to set cache #2219

elharo · 2025-04-02T12:37:23Z

…o a public constructor in DefaultDependencyResolverResult. It only hasn't failed yet because we're only passing null for this argument but there's a TODO to change that.

Alternatively, we could remove PathModularizationCache from the DefaultDependencyResolverResult.

…o a public constructor in DefaultDependencyResolverResult

desruisseaux · 2025-04-02T13:24:27Z

Alternatively, we can make the constructor package-private and add another constructor without the PathModularizationCache argument, which would delegate to the package-private constructor with a null argument.

A few months ago, a started a branch where I tried to reduce the visibility of some Maven internal classes. It required moving some classes to a common package (in this case, we would need to move DefaultProjectBuilder). I did not explored further because of conflict with other changes, but generally I would prefer going in the direction of removing some public items instead of adding more.

elharo · 2025-04-02T13:40:33Z

Yes, if we can move more classes into the same packages we can reduce visibility of a lot of things. Excessive proliferation of subpackages is a major antipattern in Java that leads to excessive visibility. It's probably second only to IDEs that mark classes and methods public by default.

gnodet · 2025-04-03T06:17:02Z

The DefaultDependencyResolverResult does not really respect the @Immutable contract which is on Result<REQ>.
It may be better refactored in a different class that would be package protected, contains the addDependency and addOutputDirectory, while the DefaultDependencyResolverResult would be kept immutable.

elharo

PTAL. Took @desruisseaux 's suggestion to make the constructor package-private and add another constructor without the PathModularizationCache argument, which delegates to the package-private constructor with a null argument.

desruisseaux · 2025-04-20T16:13:49Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java

            DependencyResolverRequest request,
            PathModularizationCache cache,
            List<Exception> exceptions,
            Node root,
            int count) {
        this.request = request;
-        this.cache = cache;
+        if (cache == null) {


This if … else block could be replaced by this.cache = Objects.requireNonNull(cache). For consistency, we may do the same for other arguments too except root which is nullable.

I don't think that's the same. cache is nullable. We need to create a cache if the argument is null, not throw an exception. Null is passed for this argument and we don't want to break those existing usages.

The new constructor should replace all the calls to the previous constructor with a null cache. We are sure that the new constructor is invoked outside the package since the old constructor became package-private. So we only need to ensure that the calls inside the package pass a non-null cache, and I think that it is already the case.

It's risky and brittle to allow null caches given the multiple locations that invoke methods on the cache and the lack of test coverage on this class. It is unsafe to allow this field to be null, even if the constructor argument is. Even if it doesn't cause an uncaught NPE now it will in the future.

Yes I agree, which is why my proposal above was to use Objects.requireNonNull.

No, I don't want to throw a NullPointerException. We can easily create a cache here..

The rational was that a cache created here is of limited use. The intend was to have a session-wide cache, which is why this method was requesting the cache in argument. The creation of a cache in the constructor was only a temporary workaround for the fact that we have not investigated where a session-wide cache should be stored.

Whatever the intent was, this field is dereferenced without null checks in multiple places. That's dangerous.

Yes, I agree that it is dereferenced without null checks, which is why the proposal above was to use Objects.requireNonNull in the constructor. Then, since the constructor become package-private as a result of the change in this pull request, we can easily verify that all invocations of this constructor pass a non-null cache. But anyway, this is not very important. We can leave it as is for now and revisit after we determined where the cache should be stored.

Pankraz76 · 2025-05-18T13:02:01Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java

            DependencyResolverRequest request,
            PathModularizationCache cache,
            List<Exception> exceptions,
            Node root,
            int count) {
        this.request = request;
-        this.cache = cache;
+        if (cache == null) {
+            this.cache = new PathModularizationCache();


could init on declaration to give context for default trading coupling for cohesion. This case is then implied default making use for only dedicated second if.

Pankraz76

might consider init on declaration to give cohesion; default - making first things first.

Pankraz76 · 2025-05-18T13:02:22Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java

            DependencyResolverRequest request,
            PathModularizationCache cache,
            List<Exception> exceptions,
            Node root,
            int count) {
        this.request = request;
-        this.cache = cache;
+        if (cache == null) {


Suggested change

if (cache == null) {

if (cache != null) {

take happy path.

Pankraz76 · 2025-05-18T13:03:52Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java

@@ -102,19 +102,27 @@ public class DefaultDependencyResolverResult implements DependencyResolverResult
     * to {@link #addDependency(Node, Dependency, Predicate, Path)}.
     *
     * @param request the corresponding request
-     * @param cache cache of module information about each dependency


private final PathModularizationCache cache = new PathModularizationCache(); L:98

https://www.reddit.com/r/learnjava/comments/ozgtxm/is_initializing_variables_at_their_declaration/

https://softwareengineering.stackexchange.com/questions/379039/what-is-the-best-practice-for-initializing-variables-outside-or-inside-of-the-c

In the particular case of this issue, creating the cache at the location where the field is defined is the opposite of the intend, because it makes the PathModularizationCache lifetime strongly coupled with the DefaultDependencyResolverResult lifetime. Such coupling makes PathModularizationCache close to useless. It was done that way in the previous version of this class only as a workaround for the fact that I didn't knew where to store a session-wide cache, but that workaround was intended to be temporary.

More details in a next comment.

my suggest would not compile. im sorry.

desruisseaux · 2025-05-18T17:32:12Z

I think that the DefaultDependencyResolverResult constructor is creating some confusion. I created a new pull request that I propose as an alternative to this one: #2347. For seeing the purpose, there is two design constraints to consider:

In order to be useful, PathModularizationCache needs to live longer than DefaultDependencyResolverResult. Any change that makes the lifetime of those two objects strongly coupled make the cache close to useless.
At the time I wrote these classes, I didn't know how session-wide caches should be managed in Maven (I still haven't investigated). Therefore, PathModularizationCache was instantiated together with DefaultDependencyResolverResult as a temporary workaround, until we learn more about how session-wide caches are managed in Maven.

The pull #2347 tries to address the same issue as this pull request, but in a way that makes more apparent that DefaultDependencyResolverResult and PathModularizationCache lifetimes are distinct in principle, and that the current coupling may be temporary.

PathModularizationCache needs to be public because it's an argument t…

22005d6

…o a public constructor in DefaultDependencyResolverResult

elharo added 5 commits April 19, 2025 09:34

Hide PathModularizationCache from public API

6d5b0bc

spotless

55c6669

remove public constructor

3ec4731

back to public constructor

4150c04

remove public

fd4dbc4

elharo commented Apr 19, 2025

View reviewed changes

elharo marked this pull request as ready for review April 19, 2025 13:51

elharo added 2 commits April 19, 2025 09:52

remove TODO

e3ac389

add TODO

36050c7

elharo changed the title ~~PathModularizationCache needs to be public because it's an argument t…~~ [MNG-8696] PathModularizationCache needs to set cache Apr 19, 2025

no null caaches

f12ffd4

elharo requested review from gnodet and desruisseaux April 20, 2025 12:30

desruisseaux reviewed Apr 20, 2025

View reviewed changes

Pankraz76 reviewed May 18, 2025

View reviewed changes

Pankraz76 suggested changes May 18, 2025

View reviewed changes

desruisseaux mentioned this pull request May 18, 2025

[MNG-8696] Hide the cache from DefaultDependencyResolverResult constructor #2347

Open

[MNG-8696] PathModularizationCache needs to set cache #2219

Are you sure you want to change the base?

[MNG-8696] PathModularizationCache needs to set cache #2219

Uh oh!

Conversation

elharo commented Apr 2, 2025

Uh oh!

desruisseaux commented Apr 2, 2025

Uh oh!

elharo commented Apr 2, 2025

Uh oh!

gnodet commented Apr 3, 2025

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pankraz76 left a comment

Choose a reason for hiding this comment

Uh oh!

Pankraz76 May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

desruisseaux commented May 18, 2025

Uh oh!

Uh oh!

Pankraz76 May 18, 2025 •

edited

Loading